HDDS-10427. Retry read wait based on policy. #6292
Conversation
ivandika3
left a comment
ivandika3
left a comment
There was a problem hiding this comment.
Thanks for catching this. LGTM overall.
I have some minor comments and nitpicks, which can be ignored / addressed.
hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/storage/BlockInputStream.java
Outdated
Show resolved
Hide resolved
| throw new IOException(e); | ||
| } | ||
| return retryAction.action == RetryPolicy.RetryAction.RetryDecision.RETRY; | ||
| if (retryAction.action == RetryPolicy.RetryAction.RetryDecision.RETRY) { |
There was a problem hiding this comment.
Just a thought: It might be nice if we can have a general RetryAction handler so that we don't need to duplicate the sleeping logic (similar codes is in AbstractDataStreamOutput#shouldRetry, GrpcOmTranport#shouldRetry, and KeyOutputStream#handleRetry)
There was a problem hiding this comment.
Yes sounds good may be we can refactor in other Jira.
| seekAndVerify(50); | ||
| byte[] b = new byte[200]; | ||
| blockInputStreamWithRetry.read(b, 0, 200); | ||
| assertThat(logCapturer.getOutput()).contains("Retry read after"); |
There was a problem hiding this comment.
Verifying debug log message is going to be brittle. But writing a test to verify time is always tricky. What I usually do is to introduce a timer class that wraps around Thread.sleep(), and we can verify the time does advance (artifically). That's going to introduce big code churn with little benefit so I'm okay with what we have now.
jojochuang
left a comment
jojochuang
left a comment
There was a problem hiding this comment.
+1. The change is similar to what's in KeyOutputStream.
ChenSammi
left a comment
ChenSammi
left a comment
There was a problem hiding this comment.
Thanks @ashishkumar50 .
5 participants
What changes were proposed in this pull request?
Currently when read fails from DN, client retries without waiting. In this PR, client will wait based on retry policy before reading again.
What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-10427
How was this patch tested?
Update existing test